Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix middleware display methods methods #687

Merged
merged 2 commits into from
Mar 15, 2017

Conversation

martinpovolny
Copy link
Member

Broken here: #590

Adding a sample nested list spec for one of the controllers.

Of cource it would be very convenient to have at least a single test for each screen (nested list in this case).

Ping @Jiri-Kremser, @karelhala

@miq-bot
Copy link
Member

miq-bot commented Mar 14, 2017

Checked commits martinpovolny/manageiq-ui-classic@7fe502c~...a623717 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks good. 👍

@jkremser
Copy link
Member

Thanks, I'll try tomorrow if everything works again. There is one another issue, now when the provider's show hamls are not being rendered, the angular controllers are not initialized properly, because it was done in those hamls. Here for instance: https://git.io/vyMc3

@martinpovolny
Copy link
Member Author

Uff, the show.html.haml partials should be rendered.

I plan to remove them but not before I handle the special cases such as this one.

@karelhala
Copy link
Contributor

karelhala commented Mar 15, 2017

@martinpovolny I can confirm that this will fix correct navigation from Middleware server to

  • Middleware Deployments
  • Middleware Datasources
  • Middleware Messagings

However navigating from Middleware Domain to MW Server groups shows nothing. However this might be our problem @Jiri-Kremser (what do you think?). But otherwise looks good 👍

Thank you Martin for providing simple test, we'll continue from this.

Oh and BTW, these Bugzillas are related to this PR:
https://bugzilla.redhat.com/show_bug.cgi?id=1431582
https://bugzilla.redhat.com/show_bug.cgi?id=1431722

@karelhala
Copy link
Contributor

Since we are speaking about show.html.haml these are not used in across any MW anymore. I've put #{binding.pry} in all of them, clicked trough whole MW section and nothing happen. It's bad since we set ng-app there to handle multiple angular modules in one page - for instance in MW server which is not hawkular provider we have ng app for deployments and JDBC driver. So the question here is, how can we now (with yhis new approach) to have multiple ng apps on one screen? This question might be more targeted to @himdel.

However I would not postpone this PR with this, and me, @Jiri-Kremser or @mtho11 can create follow-up PR with suggestions from Martin Hradil.

@jkremser
Copy link
Member

jkremser commented Mar 15, 2017

Confirming that it helped for navigation from server to its entities. But when going from Domain to server group and from server group to server is still broken. The first case is without error but nothing happens and the second case throws:

undefined method `sub_table' for nil:NilClass [middleware_server_group/show]

I think, it's the same error I saw before I pulled these changes.

@jkremser
Copy link
Member

jkremser commented Mar 15, 2017

btw. the only show haml that is being rendered according to logs is the ems_common/show.html.haml, this one renders the ems_common/_show partial in which there are dozens of cases for the @display, except the middleware_server_groups. If I add this, now I can see the error (undefined method `sub_table') also in the first case when navigating from domain to the server group :)

@martinpovolny
Copy link
Member Author

martinpovolny commented Mar 15, 2017

Oh well https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/ems_common.rb#L98 is going to be responsible for rendering the wrong template. My fault.

It would need something like unless there is a show partial for this controller use the default or moving the default_show_template to controllers that 1) include EmsCommon and 2) don't have their own show template.

It's a mess of overriding what was overridden together with default rendering by Rails.

@jkremser
Copy link
Member

lgtm, we can fix the other issues in other PR

@mzazrivec mzazrivec self-assigned this Mar 15, 2017
@mzazrivec mzazrivec added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 15, 2017
@mzazrivec mzazrivec merged commit 18093b3 into ManageIQ:master Mar 15, 2017
@israel-hdez
Copy link
Member

israel-hdez commented Mar 16, 2017

@martinpovolny To fix the wrong template rendered you may just call render. An exception will be raised and you can rescue rendering the default_show_template. The actual working code is in israel-hdez@9dc8615

That would also fix #658, but I don't know if "trying and rescuing" is a suitable solution.

@martinpovolny
Copy link
Member Author

martinpovolny commented Mar 16, 2017

@israel-hdez : that makes sense and would work. Thx!

However I would prefer to see explicitly in the code what is going to happen. Actually I am trying to move any logic out of views and have it in the controller. Eventually I'd like to remove all the show.html.haml partials and render directly the partials that are rendered from there.

Another way to fix this might be moving the default_show_template from the mixin to the controllers that actually need to render the shared partial.

Another way might be providing simple show.html.haml partials that just reference the shared partial into all the models that don't have their show. (actually reverting my earlier change where I deleted those).

I'll leave for @Jiri-Kremser to figure out the best way, add the specs and then I'll surely get back to this code as I will try to further simplify and unify the show methods. Hopefully this time not breaking the middleware controllers.

Generally I'd say that the EmsCommon is doing far too much stuff and is used in too many different ways. Probably should be split and only the relevant parts should be included by the current users of that mixin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants